Conversation
rickyrombo
commented
Mar 4, 2026
- Adds support in the auth middleware for access tokens in the Authorization: Bearer header
- Adds a request helper to extract the signer from OAuth headers
- Adds a cache for that lookup
- Adds authorize, token, revoke and "me" endpoints
There was a problem hiding this comment.
Pull request overview
Adds first-class OAuth 2.0 (PKCE-style) support to the API server, including new OAuth endpoints, Bearer-token handling in auth middleware, and token-to-signer resolution with caching.
Changes:
- Introduces
/v1/oauth/authorize,/v1/oauth/token,/v1/oauth/revoke, and/v1/oauth/meendpoints and supporting helpers (PKCE + refresh rotation + revocation). - Extends auth middleware and request signer extraction to accept
Authorization: Bearer <opaque_access_token>backed byoauth_tokens. - Adds an in-memory cache for OAuth access-token lookups and seeds/test fixtures for new OAuth tables, plus a comprehensive OAuth test suite.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| database/seed.go | Adds base seed rows for new OAuth tables used in tests/fixtures. |
| api/v1_oauth.go | Implements OAuth endpoints, PKCE validation, token issuance/refresh/revocation, and access-token cache helpers. |
| api/v1_oauth_test.go | Adds tests for token/revoke/me flows, cache behavior, JWT iat validation, and middleware integration. |
| api/server.go | Wires OAuth routes and initializes an otter cache for OAuth token lookups. |
| api/request_helpers.go | Adds signer resolution from OAuth access tokens (client_id → api_secret). |
| api/auth_middleware.go | Adds PKCE Bearer token fallback in auth middleware to set authed wallet/user from oauth_tokens. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Insert into oauth_authorization_codes | ||
| _, err = app.writePool.Exec(c.Context(), ` | ||
| INSERT INTO oauth_authorization_codes (code, client_id, user_id, redirect_uri, code_challenge, code_challenge_method, scope) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7) | ||
| `, code, clientID, int32(userId), body.RedirectURI, body.CodeChallenge, body.CodeChallengeMethod, body.Scope) | ||
| if err != nil { | ||
| app.logger.Error("Failed to insert auth code", zap.Error(err)) | ||
| return oauthError(c, fiber.StatusInternalServerError, "server_error", "Failed to create authorization code") | ||
| } |
There was a problem hiding this comment.
These handlers use app.writePool (e.g., for inserting auth codes) without guarding against writePool == nil. Other write endpoints in this codebase return a 500 with a "Database write not available"-style message when the write pool isn't configured. Consider adding the same guard at the top of the OAuth handlers to avoid nil-pointer panics on read-only deployments/configs.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // PKCE token fallback: resolve opaque Bearer token from oauth_tokens | ||
| if wallet == "" && bearerToken != "" { | ||
| if entry, ok := app.lookupOAuthAccessToken(c, bearerToken); ok { | ||
| wallet = strings.ToLower(entry.ClientID) | ||
| if myId == 0 { | ||
| myId = entry.UserID | ||
| c.Locals("myId", int(entry.UserID)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In the PKCE Bearer-token fallback, if myId is already set (via user_id query param), the code does not verify it matches the access token's user_id. That means a valid access token for user A could be used to authorize requests for user B as long as the app wallet has an approved grant for B, effectively turning per-user tokens into per-client tokens. Consider enforcing entry.UserID == myId when myId != 0 (similar to the OAuth JWT fallback), otherwise reject the request.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // --- /oauth/token (authorization_code grant) --- | ||
|
|
||
| func TestOAuthTokenAuthorizationCode(t *testing.T) { | ||
| app := emptyTestApp(t) | ||
| clientID := seedOAuthTestData(t, app) | ||
| code, codeVerifier, _ := insertTestAuthCode(t, app, clientID, 100, "write") | ||
|
|
||
| status, body := oauthPostJSON(t, app, "/v1/oauth/token", map[string]string{ | ||
| "grant_type": "authorization_code", | ||
| "code": code, | ||
| "code_verifier": codeVerifier, | ||
| "client_id": clientID, | ||
| "redirect_uri": "https://example.com/callback", | ||
| }) | ||
|
|
||
| assert.Equal(t, 200, status) | ||
| assert.True(t, gjson.GetBytes(body, "access_token").Exists()) | ||
| assert.True(t, gjson.GetBytes(body, "refresh_token").Exists()) | ||
| jsonAssert(t, body, map[string]any{ | ||
| "token_type": "Bearer", | ||
| "expires_in": float64(3600), | ||
| "scope": "write", | ||
| }) | ||
| } |
There was a problem hiding this comment.
OAuth tests cover /oauth/token, /oauth/revoke, /oauth/me, cache behavior, and middleware, but there are no tests for /v1/oauth/authorize (happy path + required-field validation + redirect URI registration + write-scope grant enforcement). Adding those would help prevent regressions in the start of the PKCE flow.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
…eived, normalize client_id to wallet address
bf44bd0 to
6b14ecf
Compare
|
@rickyrombo I've opened a new pull request, #696, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@rickyrombo I've opened a new pull request, #697, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@rickyrombo I've opened a new pull request, #698, to work on those changes. Once the pull request is ready, I'll request review from you. |
The `/v1/oauth/authorize` endpoint (entry point to the PKCE flow) had no
test coverage, leaving happy paths, validation logic, and grant
enforcement untested.
### Changes
- **`makeOAuthJWT` helper**: generates valid Ethereum personal-signed
JWTs using a fixed test private key, replicating the exact signing
format validated by `validateOAuthJWTTokenToUserId`
- **`seedOAuthTestData` update**: user 100's wallet updated to the
address derived from the test private key, enabling real JWT
verification against the DB in authorize tests
- **10 new test cases** for `POST /v1/oauth/authorize`:
- Happy path: `read` scope and `write` scope with an approved grant
- Required-field validation: missing `token`, `client_id`,
`redirect_uri`, `code_challenge`, `scope` → `400 invalid_request`
- `code_challenge_method` must be `S256` → `400 invalid_request`
- Invalid scope value → `400 invalid_request`
- Invalid/unsigned JWT → `401 access_denied`
- Unknown `client_id` → `400 invalid_client`
- `write` scope with no approved grant → `403 access_denied`
```go
// makeOAuthJWT signs a JWT with a known test private key whose wallet
// is seeded into the users table, enabling full JWT validation in tests.
func makeOAuthJWT(t *testing.T, userID int, privKeyHex string) string {
privKey, _ := crypto.HexToECDSA(privKeyHex)
message := header + "." + base64url(payload)
sig := crypto.Sign(keccak256(ethereumPrefix+message), privKey)
return message + "." + base64url(hexEncode(sig))
}
```
<!-- START COPILOT CODING AGENT TIPS -->
---
🔒 GitHub Advanced Security automatically protects Copilot coding agent
pull requests. You can protect all pull requests by enabling Advanced
Security for your repositories. [Learn more about Advanced
Security.](https://gh.io/cca-advanced-security)
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rickyrombo <3690498+rickyrombo@users.noreply.github.com>
OAuth handlers used `app.writePool` without checking for nil, causing
panics on read-only deployments where the write pool is unconfigured.
## Changes
- Added `writePool == nil` early-return guard to `v1OAuthAuthorize`,
`v1OAuthToken`, and `v1OAuthRevoke`, returning HTTP 500 `server_error` /
`"Database write not available"` via the existing `oauthError` helper
- Added same nil guard to `invalidateOAuthTokenCacheByFamily` (silent
early return, consistent with its existing `oauthTokenCache == nil`
guard)
```go
func (app *ApiServer) v1OAuthAuthorize(c *fiber.Ctx) error {
if app.writePool == nil {
app.logger.Error("Write pool not configured")
return oauthError(c, fiber.StatusInternalServerError, "server_error", "Database write not available")
}
// ...
}
```
Pattern matches the existing guard in `v1_users_developer_apps.go`.
<!-- START COPILOT CODING AGENT TIPS -->
---
💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rickyrombo <3690498+rickyrombo@users.noreply.github.com>
A valid PKCE access token for user A could authorize requests targeting
user B (via `user_id` query param) if the app wallet held an approved
grant for B — effectively making per-user tokens behave as per-client
tokens.
## Changes
- **`auth_middleware.go`**: Gate the PKCE token fallback on `myId == 0
|| entry.UserID == myId` before setting `wallet`, consistent with the
existing OAuth JWT fallback:
```go
// Before
if entry, ok := app.lookupOAuthAccessToken(c, bearerToken); ok {
wallet = strings.ToLower(entry.ClientID)
if myId == 0 {
myId = entry.UserID
c.Locals("myId", int(entry.UserID))
}
}
// After
if entry, ok := app.lookupOAuthAccessToken(c, bearerToken); ok {
if myId == 0 || entry.UserID == myId {
wallet = strings.ToLower(entry.ClientID)
if myId == 0 {
myId = entry.UserID
c.Locals("myId", int(entry.UserID))
}
}
}
```
- **`v1_oauth_test.go`**: Add
`TestAuthMiddleware_PKCEToken_UserIDMismatch` — verifies a token issued
for user 100 is rejected (403) when the request targets user 200.
<!-- START COPILOT CODING AGENT TIPS -->
---
💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rickyrombo <3690498+rickyrombo@users.noreply.github.com>